Support sidebarAbsolutePath to fix relative link and nested sidebar when I just provide one sidebar.md #1076

tangshuang · 2020-03-15T14:43:59Z

Summary

What kind of change does this PR introduce? (check at least one)

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

If adding a new feature, the PR's description includes:

A convincing reason for adding this feature
Related documents have been updated
Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

Referer to issues:

DO NOT include files inside lib directory.

anikethsaha

fix the CI error

tangshuang · 2020-03-15T15:23:52Z

@anikethsaha I’m sorry, I did not find the way to fix. Could you please have a look to this problem?

anikethsaha · 2020-03-15T15:38:22Z

its because you added a header ## in configuration.md docs. so, the fixtures need to be changed.

add sidebarAbsolutePath in array here

tangshuang · 2020-03-17T04:52:17Z

its because you added a header ## in configuration.md docs. so, the fixtures need to be changed.

add sidebarAbsolutePath in array here

Fixed. Thanks!

anikethsaha · 2020-03-19T06:33:38Z

I will review this soon

test/unit/util.test.js

trusktr · 2020-04-03T17:25:54Z

src/core/router/history/base.js

+
+    // auto alias sidebar which is set to be absolute path
+    if (config.sidebarAbsolutePath) {
+      const sidebar = config.loadSidebar || '_sidebar.md';


Do we need this sidebarAbsolutePath option? Or can we just provide an absolute path in loadSidebar? We can use a url tool to detect absolute path, like /path/to/sidebar.md, //path/to/sidebar.md, https://example.com/path/to/sidebar.md, etc.

Think about this: what happend if they set sidebarAbsolutePath to true, but it isn't an absolute path? Should it be an error? Will they be confused?

Here's an idea on how to detect absolute paths with a regex: https://stackoverflow.com/a/19709846/454780 (but we should allow paths starting with / to return true if we want.

Here's an idea on how to detect absolute paths with a regex: https://stackoverflow.com/a/19709846/454780 (but we should allow paths starting with / to return true if we want.

Yes, we should consider detecting too.

trusktr · 2020-04-03T17:32:19Z

docs/configuration.md

+It should works with `loadSidebar`. If you do not set `loadSidebar`, don't set this option any more.
+
+It works well with `relativePath` and `basePath`. You do not need to worry about the path conflict.
+


These docs don't describe what the option actually does. It doesn't mention that it can be a string. Can you please update?

I agree with @trusktr. There also seems to be some gramma thats not correct

trusktr

I think this is a good idea. But we need some changes:

just use one variable for this if possible, loadSidebar, and detect all absolute paths. Is there a reason not to use one option for this case?
Make sure the docs describe the option in full. At the moment it is not clear what to do with the option, it doesn't mention anything about the string values.

anikethsaha · 2020-04-04T02:45:46Z

thanks @trusktr for the detailed review.
just to add a point, this will be a part of v5 cause I dont think we can do any feature or enhancement release as of now, it all should be released with v5 if this gets approved.

anikethsaha · 2020-04-04T02:46:24Z

I will create a v5 branch soon or will need to discuss where to start the development and how.

arelstone

Good job on this, this is a feature that some users could defiantly use.. There is a few things i would suggest changed.

You've rearranged some imports that I don't think it necessary. The less files that is changed in a PR the easier it is to be get reviewed. I would suggest that you reverted the files that only has import changes to the previous state.

arelstone · 2020-06-15T13:37:03Z

docs/configuration.md

+It should works with `loadSidebar`. If you do not set `loadSidebar`, don't set this option any more.
+
+It works well with `relativePath` and `basePath`. You do not need to worry about the path conflict.
+


I agree with @trusktr. There also seems to be some gramma thats not correct

arelstone · 2020-06-15T13:38:55Z

packages/docsify-server-renderer/index.js

@@ -1,13 +1,13 @@
 import { readFileSync } from 'fs';
 import { resolve, basename } from 'path';
 import resolvePathname from 'resolve-pathname';
+import fetch from 'node-fetch';


I think this should be reverted to the previous state. There is no point in rearranging imports if this is the only changes in the file

This is a result of the automated husky lint process, which modifies the files. The git commit step should not leave modifications in the working tree.

arelstone · 2020-06-15T13:42:30Z

src/core/render/compiler/link.js

      !config.ignore
    ) {
-      if (href === compilerClass.config.homepage) {
+      const sidebarAbsolutePath = compiler.config.sidebarAbsolutePath;
+      const basePath =


To me the following lines has a weird formatting.

I would suggest doing this instead:

Suggested change

const basePath =

const basePath = typeof sidebarAbsolutePath === 'string'

? sidebarAbsolutePath

: router.getBasePath();

const relativeTo = isSidebar && sidebarAbsolutePath

? basePath

: router.getCurrentPath();

trusktr

Thanks @tangshuang for this idea!

We should make a function called isAbsolute, and we can use this along with out new isExternal function: if the condition isExternal(url) || isAbsolute(url) is true, we can automatically treat the URL the same way as proposed in this PR, but without having to have a new sidebarAbsolutePath option.

The more combinations of options we need to have, the more difficult it is for the user to understand and easier to get it wrong.

just to add a point, this will be a part of v5 cause I dont think we can do any feature or enhancement release as of now, it all should be released with v5 if this gets approved.

I believe for non-breaking changes we can release them in v4.x until we're ready for v5.

You've rearranged some imports that I don't think it necessary. The less files that is changed in a PR the easier it is to be get reviewed. I would suggest that you reverted the files that only has import changes to the previous state.

I agree with this in general, though as I mentioned in the other comment it is a result of the automated commit hook. EDIT: a recent fix automatically commits the changes (see #1108). Once things are in good shape, this won't happen anymore. We should make the automated tests fail for any such changes, to make them required (otherwise, let's remove the automated changes from the commit hook).

@anikethsaha @arelstone Let's take on this pull request ourselves unless @tangshuang will return to make the changes before we do.

tangshuang added 7 commits March 15, 2020 21:50

add config.sidebarAbsolutePath

024d257

fix lint

85bec56

auto alias for sidebar

6db386b

fix for plugin

14a71d2

support pass a string as sidebarAbsolutePath

8aa6a10

fix sidebarAbsolutePath

6516b76

update doc for adding sidebarAbsolutePath configuration option

33862c6

anikethsaha requested changes Mar 15, 2020

View reviewed changes

tangshuang requested a review from anikethsaha March 19, 2020 04:23

anikethsaha self-assigned this Mar 19, 2020

trusktr reviewed Apr 3, 2020

View reviewed changes

test/unit/util.test.js Show resolved Hide resolved

trusktr reviewed Apr 3, 2020

View reviewed changes

trusktr requested changes Apr 3, 2020

View reviewed changes

woss mentioned this pull request May 30, 2020

Nesting issue with more than 2 levels #1104

Closed

1 task

anikethsaha removed their assignment May 30, 2020

anikethsaha requested a review from trusktr May 30, 2020 13:32

arelstone suggested changes Jun 15, 2020

View reviewed changes

trusktr requested changes Jun 24, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sidebarAbsolutePath to fix relative link and nested sidebar when I just provide one sidebar.md #1076

Support sidebarAbsolutePath to fix relative link and nested sidebar when I just provide one sidebar.md #1076

tangshuang commented Mar 15, 2020

anikethsaha left a comment

tangshuang commented Mar 15, 2020

anikethsaha commented Mar 15, 2020

tangshuang commented Mar 17, 2020

anikethsaha commented Mar 19, 2020

trusktr Apr 3, 2020 •

edited

Loading

trusktr Apr 3, 2020

anikethsaha Apr 4, 2020

trusktr Apr 3, 2020

arelstone Jun 15, 2020

trusktr left a comment

anikethsaha commented Apr 4, 2020

anikethsaha commented Apr 4, 2020

arelstone left a comment

arelstone Jun 15, 2020

arelstone Jun 15, 2020

trusktr Jun 24, 2020

arelstone Jun 15, 2020

trusktr left a comment •

edited

Loading

		It should works with `loadSidebar`. If you do not set `loadSidebar`, don't set this option any more.

		It works well with `relativePath` and `basePath`. You do not need to worry about the path conflict.

-      const basePath =
+const basePath = typeof sidebarAbsolutePath === 'string'
+          ? sidebarAbsolutePath
+          : router.getBasePath();
+const relativeTo = isSidebar && sidebarAbsolutePath
+          ? basePath
+          : router.getCurrentPath();

Support sidebarAbsolutePath to fix relative link and nested sidebar when I just provide one sidebar.md #1076

Are you sure you want to change the base?

Support sidebarAbsolutePath to fix relative link and nested sidebar when I just provide one sidebar.md #1076

Conversation

tangshuang commented Mar 15, 2020

anikethsaha left a comment

Choose a reason for hiding this comment

tangshuang commented Mar 15, 2020

anikethsaha commented Mar 15, 2020

tangshuang commented Mar 17, 2020

anikethsaha commented Mar 19, 2020

trusktr Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trusktr left a comment

Choose a reason for hiding this comment

anikethsaha commented Apr 4, 2020

anikethsaha commented Apr 4, 2020

arelstone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trusktr left a comment • edited Loading

Choose a reason for hiding this comment

trusktr Apr 3, 2020 •

edited

Loading

trusktr left a comment •

edited

Loading